Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add rules according to notes in the documentation #806

Merged
merged 3 commits into from
May 6, 2024
Merged

Add rules according to notes in the documentation #806

merged 3 commits into from
May 6, 2024

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented May 3, 2024 •
edited
Loading

Reference to a related issue in the repository

#805

Add a description

Added rules according to existing notes in the documentation.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

@ClemensLinnhoff ClemensLinnhoff added this to the V3.7.0 milestone May 3, 2024
@ClemensLinnhoff ClemensLinnhoff self-assigned this May 3, 2024
@ClemensLinnhoff ClemensLinnhoff linked an issue May 3, 2024 that may be closed by this pull request
Check comments for mandatory fields #805
Closed
@ClemensLinnhoff ClemensLinnhoff mentioned this pull request May 3, 2024
Merged
6 tasks
@PhRosenberger PhRosenberger added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label May 6, 2024
pmai
pmai requested changes May 6, 2024
View reviewed changes
Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise, assuming that the added rules have been checked to work in the validator.

osi_common.proto Outdated Show resolved Hide resolved
osi_route.proto Outdated
@@ -42,6 +42,7 @@ message Route
//
// \rules
// is_set
// is_globally_unique
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not correct, as globally unique requires the ID to be unique across all messages of all types, whereas the current requirement is just uniqueness within all route messages (to one TP), which also makes more sense.

So the validator would need to properly handle a rule that checks for this kind of scoped uniqueness, which it currently does not; hence

Suggested change
// is_globally_unique

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmai I was too quick in committing this. On second thought: Why would anything be unique across all messages of all types? If this is really the intention, we already have a problem: moving object ID in the ground truth is set as is_globally_unique, as well as sensor id in the sensor_view. Does your interpretation mean, that if I have a sensor_id = 1 I cannot have a moving object with id = 1? This does not make sense to me.

So in my interpretation is_globally_unique means, that the uniqueness is related to that specific field. Either way, we definitely need a section in the documentation describing the rules to avoid these different interpretations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means, as you say, that all of the globally unique IDs must be unique across all messages/fields, as also implemented in the osi-validation rule implementation. I agree that for sensor_id it probably does not make sense, but it stems from the fact that we have moving object, stationary object, traffic signs and traffic lights, etc., and initially it was thought that it would be better to keep all of these IDs non-overlapping, hence the is_globally_unique rule (also ask @jdsika for the reasoning/discussions at the time).

This probably still makes sense, especially as there are still plans to merge those classes at some time. However for other ID fields (especially newly introduced ones), something more strict makes more sense. But we have no rule for this as of yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand. But then we should remove globally unique from all non-object IDs, such as sensor ID. I can make the change here.

osi_trafficcommand.proto Outdated Show resolved Hide resolved
@ClemensLinnhoff
Copy link
Contributor Author

Looks good otherwise, assuming that the added rules have been checked to work in the validator.

Yes, I checked that the rules are correctly parsed into the yml files by rules2yml of osi-validation.

@ClemensLinnhoff ClemensLinnhoff requested a review from pmai May 6, 2024 08:18
@pmai
Copy link
Contributor

pmai commented May 6, 2024

CCB 2024-05-06: Merge as-is.

Potentially further rule additions that are direct result of existing textual rules in the release could be added in maintenance/patch releases, depending on our future approach to maintenance and maintenance releases.

@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels May 6, 2024
@ClemensLinnhoff
Copy link
Contributor Author

CCB 2024-05-06: Merge as-is.

Potentially further rule additions that are direct result of existing textual rules in the release could be added in maintenance/patch releases, depending on our future approach to maintenance and maintenance releases.

Before you megre, should we remove is_globally_unique from the sensor_ids then? Or is this considered a breaking change since the rule already existed in 3.6?

@PhRosenberger
Copy link
Contributor

Before you megre, should we remove is_globally_unique from the sensor_ids then? Or is this considered a breaking change since the rule already existed in 3.6?

Let's not rush this and discuss the rules that are already there in a separate issue/PR. We are not sure if this is a quick fix for a patch or if it needs further discussion..

@ClemensLinnhoff
Copy link
Contributor Author

Before you megre, should we remove is_globally_unique from the sensor_ids then? Or is this considered a breaking change since the rule already existed in 3.6?

Let's not rush this and discuss the rules that are already there in a separate issue/PR. We are not sure if this is a quick fix for a patch or if it needs further discussion..

But if it is the way @pmai describes, a lot of trace files will definitely fail during validation. It is quite common to have sensor_id set to 0 und the host vehicle also having ID 0. I would consider the rule is_globally_unique in the sensor_id a bug.

@ClemensLinnhoff
Copy link
Contributor Author

Another thing: This is the definition of the Identifier in the documentation:

Has to be unique among all simulated items at any given time. For ground truth, the identifier of an item (object, lane, sign, etc.) must remain stable over its lifetime. Identifier values may be only be reused if the available address space is exhausted and the specific values have not been in use for several time steps. Sensor specific tracking IDs have no restrictions and should behave according to the sensor specifications.

Here it says, everything shall be unique except for Sensor specific tracking IDs.

I propose:

  • Set rule is_globally_unique in all fields that are actually supposed to be globally unique (NOT sensor ID or tracking ID for example).
  • Change description of Identifier to state that the ID is only globally unique, if specified by the rule is_globally_unique.

As already stated: I would consider this a bug in the documentation and therefore would fix this before the 3.7 release.

ClemensLinnhoff and others added 3 commits May 6, 2024 17:39
@ClemensLinnhoff @pmai
Add rules according to notes in the documentation
70d830e 
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@partner.bmw.de>
@ClemensLinnhoff @pmai
Apply suggestions from code review
6d61dce 
Co-authored-by: Pierre R. Mai <pmai@pmsf.de>
Signed-off-by: Clemens Linnhoff <clemens.linnhoff@persival.de>
@ClemensLinnhoff @pmai
Remove spaces at end of line
de45ac8 
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@partner.bmw.de>
@pmai pmai force-pushed the 805-check-comments-for-mandatory-fields branch from c00dd0e to de45ac8 Compare May 6, 2024 15:39
@pmai pmai merged commit 5ac10b3 into master May 6, 2024
10 checks passed
@pmai pmai deleted the 805-check-comments-for-mandatory-fields branch May 6, 2024 18:45
@ClemensLinnhoff
Copy link
Contributor Author

Since my previous comment has been ignored and this PR has been just merged, I have created a new issue for the documentation bug: #809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhRosenberger PhRosenberger PhRosenberger approved these changes

@pmai pmai pmai approved these changes

Assignees

@ClemensLinnhoff ClemensLinnhoff

Labels
ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Milestone
V3.7.0
Development

Successfully merging this pull request may close these issues.

Check comments for mandatory fields
3 participants
@ClemensLinnhoff @pmai @PhRosenberger